Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace ChannelConfig in Channel::new #511

Merged
merged 5 commits into from
Jan 13, 2021
Merged

Replace ChannelConfig in Channel::new #511

merged 5 commits into from
Jan 13, 2021

Conversation

vitorenesduarte
Copy link
Contributor

@vitorenesduarte vitorenesduarte commented Jan 11, 2021

Description

Instead of updating a partially complete ChannelConfig, Channel::new now constructs the ChannelConfig from scratch given its arguments. I found this API nicer to use.

All the other changes are for consistency:

  • both ConnectionConfigSide::new and ChannelConfigSide::new now have owned values as arguments
  • the order of the arguments on these methods follows the creation order: chain, client, connection, port, channel
  • ConnectionConfig::new and ChannelConfig::new are deleted as they're no longer used

I also noticed that if I use PortId::default() as a port, I get the following error: could not retrieve module from port-id: ports/defaultPort: capability not found. Is this expected?


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

relayer/src/connection.rs Outdated Show resolved Hide resolved
@ancazamfir
Copy link
Collaborator

ancazamfir commented Jan 12, 2021

I also noticed that if I use PortId::default() as a port, I get the following error: could not retrieve module from port-id: ports/defaultPort: capability not found. Is this expected?

Could you give more details on the use that shows this error?
But in any case, we should probably remove or change to transfer.

@vitorenesduarte
Copy link
Contributor Author

I also noticed that if I use PortId::default() as a port, I get the following error: could not retrieve module from port-id: ports/defaultPort: capability not found. Is this expected?

Could you give more details on the use that shows this error?

The error should show up with something like this:

    let connection_0_1: Connection = ...;
    let ordering = Order::default();
    let port_0 = PortId::default();
    let port_1 = PortId::default();
    let channel_0_1 = Channel::new(connection_0_1, ordering, port_0, port_1)?;

The full example that triggered the error is here: https://github.com/vitorenesduarte/ibc-rs/blob/reproduce_bug_rust/bbt/src/main.rs#L74-L80 (the branch is not polished; I can extract the example from there in case it's useful)

I just confirmed, and the error can also be triggered using the CLI by replacing transfer with defaultPort in the following:

rrly -c loop_config.toml tx raw chan-init ibc-0 ibc-1 connection-0 transfer transfer defaultChannel defaultChannel

@codecov-io
Copy link

Codecov Report

Merging #511 (33bfa45) into master (b1b37f5) will increase coverage by 16.0%.
The diff coverage is 56.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #511      +/-   ##
=========================================
+ Coverage    13.6%   29.6%   +16.0%     
=========================================
  Files          69     160      +91     
  Lines        3752   12797    +9045     
  Branches     1374    5026    +3652     
=========================================
+ Hits          513    3799    +3286     
- Misses       2618    8324    +5706     
- Partials      621     674      +53     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <0.0%> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <0.0%> (ø)
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/msgs.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 13.6% <0.0%> (-19.7%) ⬇️
modules/src/ics03_connection/events.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/error.rs 75.0% <ø> (+50.0%) ⬆️
... and 285 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a45567...9382282. Read the comment docs.

@ancazamfir
Copy link
Collaborator

just confirmed, and the error can also be triggered using the CLI by replacing transfer with defaultPort in the following...

Yes, that is a chain error and it is expected as there is no module on chain binding to that defaultPort. The only supported one is transfer. The relayer does not have currently a way to query valid PortId -s so if the ID is valid it just passes it in the IBC message.

Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks Vitor!

@vitorenesduarte vitorenesduarte merged commit b324dc0 into informalsystems:master Jan 13, 2021
@vitorenesduarte vitorenesduarte deleted the unify_channel_connection branch January 13, 2021 10:36
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Unify Connection::new and Channel::new

* Replace RelayPath argument in Channel::new

* Update CHANGELOG

* Change Connection::set_connection_id to take a owned value as argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants